[Image][Performance] Fast Image with Auth Token Header Caching #12648

thomas-coldwell · 2022-11-10T19:20:58Z

Details

This PR combines the draft PR from @Szymon20000 #11009 for adding fast image and the PoC auth token PR from @Beamanator #12146 to properly cache images using fast image with the auth token being passed in the headers rather than the URL.

Fixed Issues

#10792

$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Checkout this branch
Run the app on both iOS and Android
Open a chat with some image attachments
They will load in as normal the first time, but reload the app and then you will see the cached images loaded instantly in the same chat

Verify that no errors appear in the JS console

QA Steps

Open a report chat and add a new image attachment and send so the image appears in the chat (this should display a loading indicator the first time it is requested)
Reload the app and you should see the cached image displayed (with no loading indicator)
Click on an image in the chat to open the attachment modal view, close it and reopen it to see the cached image displayed without a loading state
Verify that any attachments still download as normal without any errors (including image attachments and PDFs)

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

…okenAsImageHeader

github-actions · 2022-11-10T19:21:12Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

melvin-bot · 2022-11-10T19:21:32Z

@aldo-expensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

thomas-coldwell · 2022-11-10T19:22:51Z

I have read the CLA Document and I hereby sign the CLA

aldo-expensify

Please commit suggestion to fix linter errors

src/components/ImageWithSizeCalculation.js

aldo-expensify · 2022-11-11T00:45:23Z

@thomas-coldwell can you retroactively sign your commits? please

We will also need you to complete the PR Author steps:

Check the checklist "PR Author Checklist"
Provide proof (video or picture) that this was tested in every platform

Thanks!

Asked here about it: #10792 (comment)

thomas-coldwell · 2022-11-11T11:46:21Z

Will get those sorted now @aldo-expensify cheers!

Signed-off-by: Thomas Coldwell <[email protected]>

Co-authored-by: Aldo Canepa Garay <[email protected]> Signed-off-by: Thomas Coldwell <[email protected]>

thomas-coldwell · 2022-11-11T13:57:23Z

Looks like we need to have a think about how this should work on Desktop and Web (inc. mWeb) before this PR is merged cc @Beamanator

Beamanator · 2022-11-11T18:12:44Z

Yo @thomas-coldwell so correct me if I'm wrong but I had two assumptions in regard to web / desktop:

I thought react-native-fast-image wouldn't work well for Web / Desktop, is that incorrect? For some reason I thought there was some bug that prevent us from using that lib on web / desktop
Iff there's a problem using react-native-fast-image on web i think that's fine, because we have this issue ([HOLD for payment 2024-03-29] [HOLD for payment 2024-03-26] [$250] [Feature] RNW Image: Add support for http properties in source #12603) that should fix a bug in react-native-web which will allow us to pass the auth token in an Image's header on web / desktop

thomas-coldwell · 2022-11-14T11:16:45Z

Hey @Beamanator you are correct - there is no implementation of react-native-fast-image on web so we'll have to add that ourselves. I think if we add web support to fast-image (subject to that bug fix for the header caching on RNW) then that keeps it clean and the main app can then use fast-image and has the platform implementation details hidden when its used in the Expensify app and then this PR should start working perfectly (will test and provide screenshots of those missing platforms once that support is added). Does this sound good?

thomas-coldwell · 2022-11-14T11:20:45Z

Just seen @kidroca 's comment here which matches up with my previous comment 😄

thesahindia · 2022-11-14T17:28:53Z

Sorry I am going OOO, so I won't be able to take this. Please re-assign.

OSBotify · 2022-11-24T00:51:49Z

🚀 Deployed to staging by @Beamanator in version: 1.2.31-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mountiny · 2022-11-24T16:14:03Z

@thomas-coldwell Seems like this PR has introduced a deploy blocker. Looking into this. I will prepare a revert of this PR if the solution is not obvious.

…image-caching" This reverts commit 360a167, reversing changes made to 39a68d1.

Revert "Merge pull request #12648 from margelo/@thomas/fast-image-cac…

Revert "Merge pull request #12648 from margelo/@thomas/fast-image-cac… (cherry picked from commit 01abc6f)

kidroca

Since this was reverted and I studied the logic a bit, I'll leave a review to help the next PR

I have a question about the name FastImage

Why was it decided to call App's component FastImage instead of simply Image

Due to the differences in platform code, some confusion and quirks exists

onLoad is called with one parameter structure in FastImage and another in react-native (web) Image
(FastImage onLoad | RN onLoad)
FastImage has different static methods exposed and methods like preload accept source and even source array, while in RN it's just a uri: string

When I see FastImage I would assume all the react-native-fast-image options are available to this component, but since our FastImage implementation for web/desktop so far is only a core Image component they won't work

I think it a possible improvement for a follow up PR is to

use a simple name like Image or AppImage for the component
add a specific propTypes.js interface that implement only the minimum props we use

This way we can declare the source we support ATM is number | string | { urI: string, headers: object } e.g. nothing conflicting with the differences between source structure between FastImage and regular Image

I think the change is necessary, because

we might or might not update react-native-fast-image with (full) support for web
it might take time or not be a priority to apply these updates to react-native-fast-image
so a clean interface of exactly what we use would be helpful in maintaining the cross platform App Image component

kidroca · 2022-11-25T09:34:07Z

src/components/AttachmentModal.js

@@ -115,7 +115,7 @@ class AttachmentModal extends PureComponent {
     * @param {String} sourceURL
     */
    downloadAttachment(sourceURL) {
-        fileDownload(sourceURL, this.props.originalFileName);
+        fileDownload(this.props.isAuthTokenRequired ? addEncryptedAuthTokenToURL(sourceURL) : sourceURL, this.props.originalFileName);


@Beamanator
We can update fileDownload to also work with headers.
Not sure whether any backend handling would need to be updated

making fileDownload work with headers is not as important as images, because it's used a lot less often
on the other hand the logic would be cleaner and allow automatic cache usage

src/components/ImageWithSizeCalculation.js

kidroca · 2022-11-25T10:02:33Z

src/components/ImageWithSizeCalculation.js

    }

    render() {
+        const headers = this.props.isAuthTokenRequired ? chatAttachmentTokenHeaders() : undefined;


IMO it might be better to forward isAuthTokenRequired to the Image and let the Image run this logic instead of having it repeat for any subtype of Image we have

kidroca · 2022-11-25T10:02:56Z

src/components/ImageWithSizeCalculation.js

+export default withOnyx({
+    session: {key: ONYXKEYS.SESSION},
+})(ImageWithSizeCalculation);


As pointed out in another comment - this is unused

kidroca · 2022-11-25T10:12:53Z

src/libs/chatAttachmentTokenHeaders.js

+
+let encryptedAuthToken = '';
+Onyx.connect({
+    key: ONYXKEYS.SESSION,
+    callback: session => encryptedAuthToken = lodashGet(session, 'encryptedAuthToken', ''),
+});


@Beamanator
This is not the "React Way" to deal with global or storage state
It's a bit of an antipattern actually because it doesn't integrate with component lifecycle - only a re-render would cause users of chatAttachmentTokenHeaders to re-evaluate, but the change in ONYXKEYS.SESSION would not trigger a re-render, unless the component is also subscribed to session

The "React Way" is to capture this with withOnyx so the component is subscribed to session and re-evaluates headers the moment they change
To keep it simple the withOnyx can be applied on the core Image (FastImage) component and let it perform the logic from this file

I know we use the static Onyx.connect logic for other cases, but we should distinct the cases that really need it and the cases that can use withOnyx instead
Another bad thing about the static pattern is that it is static - we can't lazy load it and the connection stays forever, while using the "React Way" would free resources

the connection can be initiated only when needed

the connection is released when the component is no longer mounted

Gooooood points, I overlooked those lifecycle points & didn't think about lazy loading / freeing resources, I'll definitely keep those in mind in the future! Next round will not do this this way ;D

OSBotify · 2022-11-25T21:34:12Z

🚀 Deployed to staging by @Beamanator in version: 1.2.32-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mountiny · 2022-11-28T12:17:12Z

cc @thomas-coldwell, thanks for the review @kidroca

mananjadhav · 2022-11-28T18:26:09Z

For the next round of QA, @Beamanator @aldo-expensify can we get a regression test suite from Testrail to ensure nothing breaks!

OSBotify · 2022-11-28T21:18:07Z

🚀 Deployed to production by @luacmartins in version: 1.2.32-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-11-28T21:40:28Z

🚀 Deployed to production by @luacmartins in version: 1.2.32-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Beamanator · 2022-11-29T10:20:25Z

@kidroca thanks for the review, I'll also request your review on the follow-up PR when it's ready :D 🙏

Why was it decided to call App's component FastImage instead of simply Image

I thought FastImage was "ok" since we had the idea to try to use FastImage for both platforms, but you have a good point that we may or may not actually be able to / desire making that happen, so I like your idea of renaming this to something like Image or something like that.

It looks like we currently often use standard react native component names when we customize them, and if we want to directly use a react native component (like TextInput) we name it RNTextInput to make it clear it's a react native component. Maybe we can follow this convention with the new Image component

@thomas-coldwell When the follow-up PR is ready can you please tag me & request a review from @kidroca as well please? 👍

thomas-coldwell · 2022-12-01T16:47:49Z

Hey @kidroca @Beamanator thanks for those points!

I agree it should be renamed and I think just Image is a good option
Also agree on passing the isAuthRequired prop down to the Image component and letting it handle applying the headers there and I'll move the withOnyx HoC down to this level too which should make it a lot more concise

I'll link the follow-up PR here shortly and make sure you are both set to review it :)

thomas-coldwell · 2022-12-01T20:11:25Z

Just addressing the issue on Android that caused this PR to be reverted and hoping you might be able to provide a bit more context around this.

The issue originated from the fact that react-native-fast-image does not call its onLoad event if the height or width of its style is set to zero (original issue DylanVann/react-native-fast-image#865) and so it would not run the size calculation in the ThumbnailImage component. Additionally, the ThumbnailImage component receives a width and height from the ImageRenderer component that pulls this from some html attributes - weirdly for the PDF thumbnails the width and height here is set to zero:

{
  src: 'https://www.expensify.com/chat-attachments/2567892380284198922/w_0f4e8046ae3f4a4af1514f1eb51f71d5c7bc39f3.jpg.1024.jpg',
  'data-expensify-source': 'https://www.expensify.com/chat-attachments/2567892380284198922/w_0f4e8046ae3f4a4af1514f1eb51f71d5c7bc39f3.jpg',
  'data-name': 'tst.jpeg',
  'data-expensify-width': '4288',
  'data-expensify-height': '2848'
},
{
  src: 'https://www.expensify.com/chat-attachments/2266530483440650648/w_f04ec020386e433bdf1ee9f230b197b059b67ec3.jpg.1024.jpg',
  'data-expensify-source': 'https://www.expensify.com/chat-attachments/2266530483440650648/w_f04ec020386e433bdf1ee9f230b197b059b67ec3.pdf',
  'data-name': 'eglspec.1.5.pdf',
  'data-expensify-width': '0',
  'data-expensify-height': '0'
}

As a result the initial width/height for the thumbnail component (specifically in the case of a PDF thumbnail) style will always be zero and this is never updated or corrected by the onLoad event firing due to that aforementioned bug in fast image. This explains why we only see it happening on the PDF thumbnail images.

I will continue to see what can be solved on the react-native-fast-image side of this as the workaround suggested does have a side effect of potential crashes with very large images on Android, but if you could let me know if the data-expensify-width being zero makes sense or if this is potentially a bug then we can make this robust end to end. cc @Beamanator @kidroca @mountiny

Beamanator · 2022-12-02T12:47:29Z

Innnteresting findings @thomas-coldwell ! I just found in our back-end code, we currently use PHP's getfilesize function to try to get the file size (here, for internal employees). And because this doesn't actually work for pdfs, we just default the height & width to 0 (here).

@thomas-coldwell what I'm planning to try next is:

Re-fork your old PR that we reverted, reproduce regression
Test setting the height & width to something non-zero for PDFs (I don't see why this would have to be dynamic, do you?)
See if everything works for me locally - If so, I'll try this change with current code to see if it breaks current stuff (I don't see how it would, but have to check).
If everything goes smoothly, I'll update the serve-side code and we can hopefully not have to deal with this non-zero width / height issue!

Beamanator · 2022-12-02T13:38:23Z

Cool I followed steps 1 & 2, tested with the old code, things looked good! I hard-coded to 30px by 30px in the test which is why it turned out so small, but I will tweak it so it looks fine & I'll test on other platforms to make sure it doesn't break anything while we're waiting for the next version of @thomas-coldwell 's PR

Screen.Recording.2022-12-02.at.3.35.22.PM.mov

mountiny · 2022-12-02T13:48:29Z

@Beamanator the code to proceed the upload is same for OldDot as well, right. does it parse receipts too? we should probably try to avoid manipulating that flow now if that is the case.

Beamanator · 2022-12-02T13:52:40Z

@vitHoracek here's the draft PR: https://github.com/Expensify/Web-Expensify/pull/35730 the code is only used when adding comments to reports, so I don't think it parses receipts 'n such, but feel free to comment on that PR if you have concerns! I'm still working on testing that it doesn't break things :D

mountiny · 2022-12-02T14:19:04Z

@Beamanator no problem, I trust you :D just wanted to raise this

thomas-coldwell · 2022-12-02T14:22:08Z

Thanks @Beamanator and @mountiny - great that this change server-side does ensure a non-zero width and height. I will get the changes in for the Fast Image onLoad event today and then these changes should work perfectly side-by-side 🙌

Beamanator · 2022-12-02T15:14:49Z

That would be very helpful, thanks @thomas-coldwell !

thomas-coldwell · 2022-12-03T07:00:43Z

Here's the draft PR @Beamanator - just travelling atm, but started doing some digging on the revert issue #13304 😃

Beamanator · 2022-12-05T11:47:58Z

Thanks @thomas-coldwell ! Will respond in that new PR 👍

Beamanator added 6 commits October 26, 2022 17:28

Pass encryptedAuthToken to image via header

e06e498

Stop sending authToken in URL

2db5ed0

Clean up lint errors

47feb3a

Fix header param passed to image source

6df9720

Merge branch 'main' of github.com:Expensify/App into beaman-passAuthT…

c6227ab

…okenAsImageHeader

Update header

b8b9a7a

thomas-coldwell requested a review from a team as a code owner November 10, 2022 19:20

melvin-bot bot requested review from aldo-expensify and removed request for a team November 10, 2022 19:21

aldo-expensify previously requested changes Nov 11, 2022

View reviewed changes

src/components/ImageWithSizeCalculation.js Outdated Show resolved Hide resolved

aldo-expensify mentioned this pull request Nov 11, 2022

[Image][Performance] Better caching for iOS images #10792

Closed

5 tasks

thomas-coldwell force-pushed the @thomas/fast-image-caching branch from 15bc0dc to df2ded5 Compare November 11, 2022 12:42

thomas-coldwell and others added 3 commits November 11, 2022 13:14

feat: Add fast-image to thumbnails & avatar

508ba92

Signed-off-by: Thomas Coldwell <[email protected]>

feat: Add fast image to attachments view

9020cb3

Signed-off-by: Thomas Coldwell <[email protected]>

Update src/components/ImageWithSizeCalculation.js

98c2590

Co-authored-by: Aldo Canepa Garay <[email protected]> Signed-off-by: Thomas Coldwell <[email protected]>

thomas-coldwell force-pushed the @thomas/fast-image-caching branch from df2ded5 to 98c2590 Compare November 11, 2022 13:15

trjExpensify requested a review from thesahindia November 14, 2022 12:02

trjExpensify mentioned this pull request Nov 14, 2022

[HOLD #10894][Image][Performance][$2000] Feature Request - Attachments should be cached after first load. It currently load from internet every time - Reported by: @parasharrajat #6527

Closed

Beamanator requested review from Beamanator and removed request for thesahindia November 14, 2022 18:52

OSBotify mentioned this pull request Nov 23, 2022

Deploy Checklist: New Expensify 2022-11-23 #12976

Closed

38 tasks

thomas-coldwell added a commit to margelo/expensify-app-fork that referenced this pull request Nov 24, 2022

Revert "Merge pull request Expensify#12648 from margelo/@thomas/fast-…

94a90e1

…image-caching" This reverts commit 360a167, reversing changes made to 39a68d1.

mountiny mentioned this pull request Nov 24, 2022

Revert "Merge pull request #12648 from margelo/@thomas/fast-image-cac… #13010

Merged

96 tasks

luacmartins added a commit that referenced this pull request Nov 24, 2022

Merge pull request #13010 from margelo/@thomas/revert-fast-image

01abc6f

Revert "Merge pull request #12648 from margelo/@thomas/fast-image-cac…

OSBotify pushed a commit that referenced this pull request Nov 24, 2022

Merge pull request #13010 from margelo/@thomas/revert-fast-image

22b0b6f

Revert "Merge pull request #12648 from margelo/@thomas/fast-image-cac… (cherry picked from commit 01abc6f)

kidroca reviewed Nov 25, 2022

View reviewed changes

mountiny assigned thomas-coldwell Nov 25, 2022

OSBotify mentioned this pull request Nov 25, 2022

Deploy Checklist: New Expensify 2022-11-25 #13059

Closed

21 tasks

aldo-expensify mentioned this pull request Nov 28, 2022

[$2000] Large image doesn't zoom well in Android and gets blurry #12893

Closed

thomas-coldwell mentioned this pull request Dec 3, 2022

[Image][Performance] Fast Image with Auth Token Header Caching (revert follow-up) #13304

Merged

60 tasks

Santhosh-Sellavel mentioned this pull request Jan 30, 2023

adding carousel to attachment modal #9279

Merged

52 tasks

[Image][Performance] Fast Image with Auth Token Header Caching #12648

[Image][Performance] Fast Image with Auth Token Header Caching #12648

Conversation

thomas-coldwell commented Nov 10, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

github-actions bot commented Nov 10, 2022 • edited Loading

melvin-bot bot commented Nov 10, 2022

thomas-coldwell commented Nov 10, 2022

aldo-expensify left a comment

Choose a reason for hiding this comment

aldo-expensify commented Nov 11, 2022 • edited Loading

thomas-coldwell commented Nov 11, 2022

thomas-coldwell commented Nov 11, 2022

Beamanator commented Nov 11, 2022 • edited Loading

thomas-coldwell commented Nov 14, 2022

thomas-coldwell commented Nov 14, 2022

thesahindia commented Nov 14, 2022

OSBotify commented Nov 24, 2022

mountiny commented Nov 24, 2022

kidroca left a comment • edited Loading

Choose a reason for hiding this comment

kidroca Nov 25, 2022

Choose a reason for hiding this comment

kidroca Nov 25, 2022

Choose a reason for hiding this comment

kidroca Nov 25, 2022

Choose a reason for hiding this comment

kidroca Nov 25, 2022

Choose a reason for hiding this comment

Beamanator Nov 29, 2022

Choose a reason for hiding this comment

OSBotify commented Nov 25, 2022

mountiny commented Nov 28, 2022

mananjadhav commented Nov 28, 2022 • edited Loading

OSBotify commented Nov 28, 2022

OSBotify commented Nov 28, 2022

Beamanator commented Nov 29, 2022

thomas-coldwell commented Dec 1, 2022 • edited Loading

thomas-coldwell commented Dec 1, 2022 • edited Loading

Beamanator commented Dec 2, 2022 • edited Loading

Beamanator commented Dec 2, 2022

mountiny commented Dec 2, 2022

Beamanator commented Dec 2, 2022

mountiny commented Dec 2, 2022

thomas-coldwell commented Dec 2, 2022

Beamanator commented Dec 2, 2022

thomas-coldwell commented Dec 3, 2022

Beamanator commented Dec 5, 2022

thomas-coldwell commented Nov 10, 2022 •

edited

Loading

github-actions bot commented Nov 10, 2022 •

edited

Loading

aldo-expensify commented Nov 11, 2022 •

edited

Loading

Beamanator commented Nov 11, 2022 •

edited

Loading

kidroca left a comment •

edited

Loading

mananjadhav commented Nov 28, 2022 •

edited

Loading

thomas-coldwell commented Dec 1, 2022 •

edited

Loading

thomas-coldwell commented Dec 1, 2022 •

edited

Loading

Beamanator commented Dec 2, 2022 •

edited

Loading